Repository navigation
fix(core): Safari keeps playing edits' audio without seeks or rate changes - #6888
Conversation
835c49b to
3ec4752
Compare
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 1 important · 0 nits · head 65493212
- blocker — PR body has no
fails without the fix:line. The Tests section saysmediaSteer.test.tsandmedia.test.tsfail on main; name the exact tests on that line (e.g.mediaSteer.test.ts› "never steers audio: …",media.test.ts› "leaves a playing audio lagging past the sync tolerance at its rate, unseeked").
Scope
Read the full diff: mediaSteer.ts, the markMediaRestarted removals in media.ts (no other callers left), unit tests, webkit-stops.ts safaridriver path and --assert, the new macOS workflow (actions SHA-pinned; brew install ffmpeg unpinned, same as the existing Safari perf job). CI at review time: CI, regression, Windows, Player Safari audio/perf still running.
| if (settled && lag <= MEDIA_SYNC_TOLERANCE_SECONDS) catchingUp.delete(el); | ||
| const seekable = !catchingUp.has(el) || lag > MEDIA_HARD_SYNC_SECONDS; | ||
| if (audioOnly && settled && lag > AUDIO_STEER_LIMIT_SECONDS && seekable) { | ||
| if (audioOnly) { |
There was a problem hiding this comment.
important — this loosens audio sync to 0.65 s in every browser, not just WebKit. In the PR's own Chrome numbers, main already had 0% frozen and 2 seeks, so Chrome users get up to 0.65 s audio/picture drift (≈16 frames — visible lip-sync slip on a voiceover track over avatar video) and nothing in return. clock.safariAudio.test.ts is loosened from 0.1 to 0.65 to match. Consider gating the 0.65 s threshold to WebKit, or keeping a tighter bound elsewhere. You already asked whether to add a lower bound, so this is the answer for non-WebKit browsers.
There was a problem hiding this comment.
Gated: WebKit keeps 0.65 s, every other browser seeks audio past 0.1 s (no rate nudges for audio). Chrome 156, 3 plays: still 2 audio seeks, 0% frozen. Pinned by new tests in mediaSteer.test.ts. The PR body now has the fails-without-the-fix line.
Edit accuracy: accurate 271 (base branch 271), smooth 105 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (2)
|
There was a problem hiding this comment.
Verdict: APPROVE · 0 blocker · 0 important · 0 nits · 65493212 → 7992aba1
2 resolved, 0 open, 0 new. The fails without the fix: line now names the tests. The 0.65 s audio-seek limit now applies only when isWebKit() is true, so Safari and iOS browsers (CriOS/EdgiOS included) get 0.65 s, while Chrome, Edge, Android and Firefox keep 0.1 s, covered by the new Chrome test. Player Safari audio/perf passed on the previous head. The non-required Comments ratchet fails on this head because of the new doc comment on isWebKit in mediaClock.ts; CI, CodeQL, Windows and regression were still running at review time.
7992aba to
1c2531e
Compare
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · 7992aba1 → 1c2531ee
0 resolved, 1 open, 1 new: restoring main's steering outside WebKit is right, but the stricter isWebKit regex now leaves embedded WKWebViews out of the fix (inline).
| /AppleWebKit.*(Safari|Version)\//.test(userAgent) && | ||
| !/Chrom(e|ium)\/|Edg\/|Android/.test(userAgent) |
There was a problem hiding this comment.
blocker — requiring Safari/ or Version/ drops every embedded WKWebView. The default WKWebView user agent is … AppleWebKit/605.1.15 (KHTML, like Gecko) Mobile/15E148, with no Safari/ or Version/ (in-app browsers and native-app webviews on iOS and macOS). Those players take main's steering path again and keep the seek loop this PR fixes. The PR's own numbers come from a WKWebView, and the Safari CI job can't catch this because Playwright WebKit and real Safari both send Safari/. To keep jsdom out, exclude it by name:
| /AppleWebKit.*(Safari|Version)\//.test(userAgent) && | |
| !/Chrom(e|ium)\/|Edg\/|Android/.test(userAgent) | |
| /AppleWebKit/.test(userAgent) && | |
| !/Chrom(e|ium)\/|Edg\/|Android|jsdom/.test(userAgent) |
Also add a test with a bare WKWebView user agent.
There was a problem hiding this comment.
Fixed: isWebKit() is now AppleWebKit and not Chrome/Chromium/Edg/Android, no Safari token needed. mediaSteer.test.ts has a case with a default iPhone WKWebView user agent (no Safari/, no Version/), which fails on the previous regex. The test DOMs (jsdom in core, happy-dom in player) say AppleWebKit too, so both vitest configs now pin a Chrome user agent instead of the detector learning about them.
721a8e5 to
8165392
Compare
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 resolved, 0 open, 1 new · 721a8e56 → 81653923
✅ isWebKit now matches embedded WKWebViews, and a bare-WKWebView test covers it; the test setup pins a Chrome user agent, so jsdom no longer counts as WebKit.
- blocker — the required check Studio and player captures fails on this head: the
packages/player/vitest.config.tschange puts the PR under the capture rule, and the body has no## Before/## After. Body-only fix: add a## No visible changesection (test-config only) and re-run the check. The code is fine; I'll approve once that check passes.
|
The capture check passes on 8165392: the body now has Before and After sections with images (run 41260338657, after the body edit). The failed entry in the rollup is the earlier run on the same head before the body was fixed. |
There was a problem hiding this comment.
Verdict: APPROVE · 1 resolved, 0 open, 0 new · 81653923 (no code change)
✅ The body has ## Before/## After with images, and the fresh PR captures run 41260338657 passed on this head; CI, regression and Windows passed alongside it. Heads-up: the re-run of the stale run 41256465022 (attempt 2, 02:49 UTC) failed again because it replays the old event's body without Before/After. That leaves the newest Studio and player captures result red. Re-run 41260338657, or edit the body, to make the latest result green.
What
In WebKit, playing audio is no longer steered by rate and is seeked only when it holds more than 0.65 s behind the playhead. Every other browser keeps today's steering untouched; the player's audio proxies share the rule. The same PR adds the check that holds it:
webkit-stops.ts --browser safariplays the 3-take and 10-take edits in the real Safari through safaridriver, and a macos-latest workflow runs both edits with--assert.Why
WebKit reports a just-played or just-seeked
<audio>'s start time for 150 to 400 ms before output begins. Any seek or rate change aimed at that lag lands the audio behind by the same amount again. #6854 cut the loop but real Safari still froze the sound 10 to 99% of the time. Remotion tolerates the lag instead of steering it (never a rate change, seeks at 0.65 s); this does the same.Numbers
Real WebKit (macOS 26.5 system WebKit in a WKWebView), the 3-take edit (24.8 s), 5 plays each, playhead and every playing
<audio>sampled each frame. Frozen = share of playing-audio timecurrentTimestood still for over 50 ms. Medians, with the range in brackets.<Player>Media served over a slow link in this run. With the media on the same machine (5 plays): main 6 seeks and 11.5% frozen, this PR 2 seeks and 3.2%, Remotion 3 seeks and 0%. What remains in this PR is WebKit's own start delay at each of the 3 audio starts (about 250 ms each), which Remotion does not show on the same file. Not fixed here.
10-take edit (53 s), media on the same machine, 5 plays: main 18 seeks (16 to 27) and 13.9% frozen, this PR 9 seeks and 4.5%. Remotion's page only builds the 3-take edit, so it has no 10-take row.
Chrome 156 (Playwright), same edit, 3 plays each: main and this PR both 2 audio seeks, 0% frozen, at most 1 playhead stop under 80 ms.
Before
One play of the 3-take edit in WebKit (the median of 5 by frozen time), drawn from the audio element's sampled time; red is the audio not advancing for more than 50 ms.
After
Tests
mediaSteer.test.ts> "never steers WebKit audio: it is seeked only once its lag holds past 0.65 s";clock.safariAudio.test.ts> "plays through without seeking the audio it follows" (its 0.65 s bound, with a Safari user agent).tscclean.--assertfails a run above 15 audio seeks or 15% frozen.Notes
In WebKit audio can now sit up to 0.65 s behind the picture where it used to be pulled back within 0.1 s. That is the trade Remotion makes. Other browsers are unchanged.